Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 3 changed, 0 removedBuild ID: 209654623218b069d4b06a3a URL: https://www.apollographql.com/docs/deploy-preview/209654623218b069d4b06a3a
|
This comment has been minimized.
This comment has been minimized.
carodewig
left a comment
There was a problem hiding this comment.
I'm concerned about this - my gut says that this is a breaking change and should go into router v3 with the OTEL library upgrade, rather than the next v2 feature release.
The change is called out as a breaking change in the Rust OTEL library (v0.29.0):
Breaking Export configuration done via code is final. ENV variables cannot be used to override the code config. Do not use code based config, if there is desire to control the settings via ENV variables. List of ENV variables and corresponding setting being affected by this change.
Happy to be overruled! Just wanted to voice the concern to promote discussion
|
@carodewig thank you for your input. Essentially those variables never should be permitted because it will on the majority of the time impact the customer, because either insights or external APM will stop working (or both if using blocked url values). We have got several cases where the customer were running it for a while not actually noticing since they do not always use Insights that much. And support frequently spend a good amount of back and forth for those problems that are caused by setting those variables. Several time clients do not even know they are set if they are using OTEL-compliant pre made deployments., and do not have access or knowledge to even validate them on runtime. Even after the warning was added, the number of support cases related to this did not drop. I'm happy to get together if you feel we need to discuss this further to make a decision, but I think it's in the users best interest that we don't allow the application to run with something we know it only causes problems and in our setup is never really useful. I will also update documentation to make this clear if it's approved. |
carodewig
left a comment
There was a problem hiding this comment.
Thanks for the explanation, Marcelo! Given your comments I'm okay with prohibiting the use of these environment variables, but I think that needs to be updated in the docs as well (you can make the changes in docs/source/routing in this repo).
I've left a few comments below and also created a PR to explain some implementation suggestions that didn't fit well with the github review interface: #8924
NB: I think you meant to cc @abernix rather than the other Jesse :)
Co-authored-by: Caroline Rodewig <16093297+carodewig@users.noreply.github.com>
Introduced startup validation to block Apollo Router startup when certain OpenTelemetry (OTEL) environment variables are set. Removed previous warning about OTEL variable precedence and added tests for the new validation logic.
|
Thank you for all your assistance @carodewig, I have merged your code changes and modified the changelog according to your review. |
Clarified the impact of OpenTelemetry environment variables on Router telemetry settings, including version-specific behavior.
Added warnings about OpenTelemetry environment variables overriding Router telemetry settings and their impact on Apollo Router versions.
Updated warning to caution and clarified environment variable requirements for Apollo Router.
Applied suggestions from AI review 20260226-911566dd-8915-9b26fa637ce7be6c: - docs/source/routing/observability/graphos/graphos-reporting.mdx:72: Ensure there are two newlines between the admonition component and its content. - docs/source/routing/observability/graphos/graphos-reporting.mdx:74: Use hyphens (-) instead of asterisks for unordered list items. - docs/source/routing/observability/graphos/graphos-reporting.mdx:75: Use hyphens (-) instead of asterisks for unordered list items. - docs/source/routing/observability/graphos/graphos-reporting.mdx:76: Use hyphens (-) instead of asterisks for unordered list items. - docs/source/routing/observability/graphos/graphos-reporting.mdx:78: Remove 'the' before 'Router' as it is a standalone product name. - docs/source/routing/observability/graphos/graphos-reporting.mdx:80: Remove 'the' before 'Router' as it is a standalone product name.; Do not use bol... - docs/source/routing/observability/graphos/graphos-reporting.mdx:82: Remove 'the' before 'Router' as it is a standalone product name.; Do not use bol... - docs/source/routing/observability/graphos/graphos-reporting.mdx:83: Use the imperative mood for instructions to be more direct.; Remove 'the' before... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:74: Introduce the list with a sentence or fragment that ends in a colon. - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:80: Do not use an article before a standalone product name like Router. - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:82: Do not use an article before a standalone product name like Router.; Do not use ... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:84: Do not use an article before a standalone product name like Router.; Do not use ... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:85: Use the imperative mood for instructions.; Do not use an article before a standa... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:70: Introduce the list with a sentence or fragment that ends in a colon. - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:76: Do not use an article before a standalone product name like Router. - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:78: Do not use an article before a standalone product name like Router.; Do not use ... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:80: Do not use an article before a standalone product name like Router.; Do not use ... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:81: Use imperative verbs for instructions.; Do not use an article before a standalon... Review: #8915 Triggered by: marcelo.martins@apollographql.com
carodewig
left a comment
There was a problem hiding this comment.
A few style suggestions - I'd suggest going through the AI style review! My comments apply to each of the doc files although I just put them on the first
|
Thank you @carodewig for encouraging the use of the AI review! @OriginLeon I'll also point to this comment that contains a link to the review log. You can review and bulk-apply suggestions there. Feel free to post in our |
Applied suggestions from AI review 20260227-911566dd-8915-6d1a91eba47c09e1: - docs/source/routing/observability/graphos/graphos-reporting.mdx:83: Remove 'the' before the standalone product name 'Router'.; Ensure there are two ... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:81: Remove the article 'the' before the standalone product name 'Router'.; Use an au... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:80: Use "Apollo Router" instead of just "Router" to maintain clarity and reader-cent... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:85: Remove the article 'the' before the standalone product name 'Router'.; Use an au... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:80: Use the imperative 'Be aware' and address the reader with 'your router'.; Remove... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:82: Frame the content relative to the reader using 'your router'.; Remove the articl... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:84: Frame the content relative to the reader using 'your router'.; Remove the articl... Review: #8915 Triggered by: marcelo.martins@apollographql.com
Applied suggestions from AI review 20260227-911566dd-8915-863ec55aa75fef4e: - docs/source/routing/observability/graphos/graphos-reporting.mdx:79: Use reader-centric language like "your router" instead of just "Router".; Use an... - docs/source/routing/observability/graphos/graphos-reporting.mdx:81: Use reader-centric language like "your router's" to clarify ownership.; Do not u... - docs/source/routing/observability/graphos/graphos-reporting.mdx:83: Use reader-centric language like "your router" instead of the impersonal "router... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:81: Use reader-centric language like 'your router' instead of 'Router's'.; Use an ar... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:83: Use reader-centric language like 'your router' instead of 'Router'.; Remove 'the... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/metrics-exporters/otlp.mdx:85: Use reader-centric language like 'your router' instead of 'router'.; Remove 'the... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:76: Frame content relative to the reader's organization or tools using 'your'.; Remo... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:78: Use 'your' to clarify ownership of the router settings.; Remove the article 'the... - docs/source/routing/observability/router-telemetry-otel/telemetry-pipelines/trace-exporters/otlp.mdx:80: Use 'your' to address the reader's instance of the router.; Remove the article '... Review: #8915 Triggered by: marcelo.martins@apollographql.com
mabuyo
left a comment
There was a problem hiding this comment.
Good on the docs side now!
Applied suggestions from AI review 20260227-911566dd-8915-b609862e5ed41517: - docs/shared/otel-envvars-caution.mdx:3: Remove the possessive 'your' before the product name 'router'.; Use present tens... - docs/shared/otel-envvars-caution.mdx:9: Remove the possessive 'your' before the product name 'router'.; Use plain text f... - docs/shared/otel-envvars-caution.mdx:13: Use reader-centric language by adding 'your' to 'environment'.; Remove the posse... Review: #8915 Triggered by: marcelo.martins@apollographql.com
Co-authored-by: Caroline Rodewig <16093297+carodewig@users.noreply.github.com> Co-authored-by: apollo-librarian[bot] <212934294+apollo-librarian[bot]@users.noreply.github.com>
Co-authored-by: Caroline Rodewig <16093297+carodewig@users.noreply.github.com> Co-authored-by: apollo-librarian[bot] <212934294+apollo-librarian[bot]@users.noreply.github.com>
This pull request introduces a new startup validation in the Apollo Router to block startup if certain OpenTelemetry (OTEL) environment variables are set, as part of a new policy (ROUTER-1609). It also removes a previous warning about OTEL variable precedence and adds comprehensive tests for the new validation logic.
OTEL Environment Variable Validation
FORBIDDEN_OTEL_VARSlisting OTEL environment variables that must not be set:OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, andOTEL_EXPORTER_OTLP_METRICS_ENDPOINT(apollo-router/src/executable.rs).Opt::validate_otel_envs_not_present()to check for the presence of forbidden OTEL variables, returning an error if any are set and blocking router startup (apollo-router/src/executable.rs) [1] [2].Removal of Deprecated Warning
OTEL_EXPORTER_OTLP_ENDPOINTwas set, since the new validation now blocks startup instead of just warning (apollo-router/src/executable.rs).Testing
apollo-router/src/executable.rs).Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩